-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Auth] Handle corrupt keychain value resulting from incomplete v11 port #13643
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Risks
- The added error handling will delete the iCloud entry from the buggy versions that is corrupting the non-iCloud bucket. Since the corrupt iCloud entry isn't really be synced across devices, it needs to be removed because it's blocking non-iCloud entries from being written. To avoid having to delete this, it would result in trickier keychain migration code that may things more complicated and prone to error.
I think the decision not to implement the trickier keychain migration code is reasonable since the situation only affects users of apps that upgraded to Firebase 11.0-11.2, deployed to the App Store, and the user had downloaded that version.
Also, AFAICT, it's annoying rather than breaking since the issue will now get fixed when the user signs in again.
LGTM on green (might be good to wait for a 2nd opinion from Paul though). I kicked off another CI run since I think the previous failure was a flake.
FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing it all down for future us!
This should be covered in the changelog for entry for the previous PR that added back the missing key.
Addressing the second case described in #13584 (comment):
This was tricky enough where I thought it better for the explanation to be embedded next to the code for future us. See diff.
Alternatives considered
Risks
Test observations
Current behavior on buggy versions:
Fix #13584